Handle all cachekey regex capture groups - #13653

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Requesting changes, on one narrow point. The core fix is right and I verified it against PCRE2 directly rather than reading it off the code: with a 10-pair match buffer, pcre2_match() returns 10 for a 9-group pattern but 0 for 10 and 12 groups, meaning "matched, ovector too small". The old loop then ran zero times, leaving captures empty, and Pattern::process() did captures.begin() + 1 on an empty vector. Sizing the buffer from get_capture_count() + 1 fixes the cause rather than the symptom, and the replay test verifies full cache keys with as: equal while bracketing the boundary at 9, 10 and 12 groups. Good test design.

The $N bound in replace() is still wrong, and you are already editing that line

matches.size() is the pcre2_match() return value, so it is one past the highest group that participated, not the number the pattern defines. For /(.*-)(\d+)(\?.*)?$/$1$3/ against a subject with no query string, the trailing group does not participate, $3 looks out of range, and the whole replacement is rejected with "invalid reference in replacement string". The element is then silently dropped from the cache key.

This predates your patch, so I would normally send it off as its own issue. I am asking for it here because the change at line 260 swaps matchCount for matches.size(), and those two are equal once the buffer is sized correctly, so that edit is a no-op sitting on top of the actual bug. Fixing the bound is less work than leaving it half-corrected.

There is a merged in-tree precedent to copy: #13352 fixed the identical bug in the prefetch plugin. It validates $N once at config-load time against the pattern's real capture count, then substitutes an empty string at match time for a group that did not participate, per PCRE2 semantics. plugins/prefetch/pattern.cc is worth reading side by side, and the comment above its substitution explains the ""-rather-than-default-view detail that keeps data() non-null.

For cachekey the insertion point is cleaner than it was for prefetch, since Pattern::compile() already does both the regex compile and the $N token parse in one function. At the end of it:

int32_t const captureCount = _re.get_capture_count();
if (captureCount < 0) {
  CacheKeyError("failed to get capture count for pattern '%s'", _pattern.c_str());
  return false;
}
for (int i = 0; i < _tokenCount; i++) {
  if (_tokens[i] > captureCount) {
    CacheKeyError("invalid reference $%d in replacement '%s': pattern defines only %d group(s)", _tokens[i],
                  _replacement.c_str(), captureCount);
    return false;
  }
}

Then drop the runtime validation loop at line 259 and clamp the lookup:

std::string_view capture = (replIndex < matches.size()) ? matches[replIndex] : std::string_view{""};

That also closes something the current diff opens on its own: get_capture_count() returns -1 when pcre2_pattern_info() fails, and -1 + 1 == 0 into the uint32_t size parameter gives a single-pair buffer, which captures group zero only and yields a silently wrong cache key. The _re.empty() check above makes it unlikely rather than impossible. The guard above handles it.

Why this is worth the extra lines

Both loop-bound changes in the PR are currently unfalsifiable. With the buffer sized correctly, matchCount == matches.size() on every successful match, so reverting either one leaves all six replay cases green. Fixing the bound is what makes them load-bearing, and the optional-group case is what tests them. Your replay file already exists, so it is one more remap rule and one more transaction.

Notes, no action needed

Three of the six cases are regression tests. The 9-group, whole-pattern and no-match cases pass on unpatched master, which is fine, they are boundary controls, and the 9-group one sits exactly at the old capacity so it is worth keeping.

The 12-group case is the first cachekey test to push RegexMatches past its 400-byte inline buffer onto ::malloc, so this adds a per-transaction heap allocation for patterns with 10 or more groups. Worth a line in the description, and worth running that case under ASan.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

✅ Approve

Both asks from my last round are done, and the description covers the two notes I said needed no action.

Verified: the $N bound moved to config-load time

compile() now caches _captureCount and validates every $N against it once, so an out-of-range reference fails the remap rule instead of silently dropping an element from the cache key at match time. The runtime loop is gone and the lookup clamps with std::string_view{""}, which keeps data() non-null the way the prefetch precedent does.

I checked the failure path rather than assuming it: a failed init() sets status = false in configs.cc, which rejects the rule, so the _captureCount < 0 state cannot reach capture() or replace(). That closes the -1 + 1 == 0 hole the earlier diff opened.

Caching the count in the object is better than what I suggested, since it also takes get_capture_count() off the per-match path.

Verified: the loop-bound changes are now falsifiable

This was the part I cared about. ^(a)(b)?(c)?$ with $1-$2-$3 across a, ab, ac, abc is exactly the case that distinguishes a correct bound from a broken one, and ac is the load-bearing member: group 2 is absent while group 3 participates, so a clamp that merely truncated would produce the wrong key rather than a--c. It is covered twice, in pattern_test.cc and as four transactions in capture.replay.yaml with as: equal on the full key.

Your note that the new coverage fails against the pre-review implementation is the sentence I was looking for. That is what makes these tests load-bearing rather than decorative.

I also traced the case the test depends on: RegexMatches::operator[] returns "" both for an index past _size and for a group whose ovector pair is PCRE2_UNSET, so a non-participating group inside the populated range is handled too, not only one past the end.

Agreed: the twelve-group case

Covered in the unit tests for both capture and replace, the heap allocation is called out in the description, and it ran under ASan. That was the whole of what I asked for.

CI is 14 of 14.